Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: spend fidelity bond #556

Merged
merged 32 commits into from
Dec 13, 2022
Merged

feat: spend fidelity bond #556

merged 32 commits into from
Dec 13, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Nov 1, 2022 •
edited
Loading

Partly addresses #547

First iteration of moving a fidelity bond after it expired.
Feedback is highly welcome, especially in regard to wording.

A follow-up PR for "renewing" the FB is in the making (also part of #547).

First attempts of this feature were based on the "Create FB" flow:

  • Select target jar
  • Freeze non-participating UTXOs manually
  • Unfreeze the Fidelity Bond UTXO
  • Display payment info and confirm
  • Send FB UTXO via direct-send
  • Restore state of previously frozen UTXOs

While this method gives users a better understanding of what is going on, it was annoying, resulted in so much code and just felt bloated.

Now it is just:

  • Select target jar
  • Display payment info and confirm

With the current approach, the necessary intermediate steps are hidden from the user. This makes for a much nicer UX. Any input on this is highly appreciated.

Also, the resulting method has been generalized: Sending any amount of specific UTXOs to a generic destination (internal or external). This can be reused in the upcoming PR when renewing the FB and also elsewhere.

Other stuff

  • disallow unfreezing of expired fidelity bonds from JarDetailsOverlay component
    • this is necessary as unfrozen, expired FB cannot take part in taker operations (when starting a maker operation they are auto-frozen by JM, but unfortunately not for taker operations)
    • Fidelity Bond UTXOs will always show the lock icon
  • externalized PaymentConfirmModal to be used from other components than Send
  • prepare and lay ground for More detailed error messages #252 to improve error messages coming from the backend
  • order fidelity bond UTXOs by value and locked state
  • prevent Balance component from unnecessary re-renders (+handle possible NaN values)

Note: Spending an expired fidelity bond UTXO can still fail if the median time of the chain is still lower than the locktime of the bond. There is currently no way to verify if it is successfully spendable and a bond will always be marked as "expired" if its locktime is in the past.

How to test?

Create an expired fidelity bond (only possible when starting via npm run dev:start) and try moving it into a jar.
Also note, that only the actual fidelity bond UTXO should be spent, all other UTXOs should be frozen and not be moved. Once the payment is done (regardless of success), UTXOs should have their original state again.

📸

@theborakompanioni theborakompanioni added enhancement New feature or request concept Wild idea, or too many details unknown yet labels Nov 1, 2022
@theborakompanioni theborakompanioni self-assigned this Nov 1, 2022
@theborakompanioni theborakompanioni force-pushed the spend-fidelity-bond branch 3 times, most recently from 87bacb0 to 846cee6 Compare November 2, 2022 11:59
@theborakompanioni theborakompanioni marked this pull request as ready for review November 2, 2022 13:53
@theborakompanioni theborakompanioni marked this pull request as draft November 2, 2022 14:24
@theborakompanioni theborakompanioni marked this pull request as ready for review November 2, 2022 15:23
@editwentyone
Copy link

Nice work so far!

Is the source jar selectable to move the fb utxo inside there? If not, Maybe it should be grayed out?

„Payment without privacy improvements“ I think will stick stay with „moving without privacy improvements“ because we are not paying anything in the users eye

I like it. Where is this „moving“ icon inside the button from?

@theborakompanioni
Copy link
Collaborator Author

Nice work so far!

Is the source jar selectable to move the fb utxo inside there? If not, Maybe it should be grayed out?

Yes. You can move the expired FB anywhere.

„Payment without privacy improvements“ I think will stick stay with „moving without privacy improvements“ because we are not paying anything in the users eye

I was thinking of completely removing this phrase, as users might be confused and it is not really needed here. What do you think?

I like it. Where is this „moving“ icon inside the button from?

"Transfer" icon on https://bitcoinicons.com
I know that there is no icon in your template. Can also be removed, if you say so!

@editwentyone
Copy link

All good so far. Will have a look asap from my desktop. But overall you nailed it. Maybe we should rephrase it to something like: make fb usable again… bla bla

so the user gets the context that before that it’s not possible to access the funds

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Nov 4, 2022 •
edited
Loading

All good so far. Will have a look asap from my desktop. But overall you nailed it. Maybe we should rephrase it to something like: make fb usable again… bla bla

so the user gets the context that before that it’s not possible to access the funds

That's true.. maybe "Move to Jar" is misleading as it already is in Jar A, it is just not able to take part in a taker/maker operations.
Any input is highly appreciated. Maybe just something plain and simple like "Spend"?

@theborakompanioni
dev: ability to append content to ExistingFidelityBond component
4a0d3a3
@theborakompanioni
send-fb - wip
bcb09cb
@theborakompanioni
refactor: remove most code and revert to simplest method
c0bb836
@theborakompanioni
refactor: rename SpendFidelityBond to MoveFidelityBondModal
be21ad4
@theborakompanioni
refactor: rename successfulSendData to txInfo
71aac85
@theborakompanioni
dev: report to parent if data has changed and should be reloaded
466d1b2
@theborakompanioni
refactor(i18n): add global.errors keys
5bf8795
@theborakompanioni
chore(i18n): proper translation strings in move fb modal
d35fb58
@theborakompanioni
fix: keep on unfreezing previously frozen coins on errors
846cc66
@theborakompanioni
fix: improve resolving of api error messages
0672475
@theborakompanioni
dev: freeze previously unfrozen fidelity bond on error
a5de13d
@theborakompanioni
refactor: use generic 'spendUtxosWithDirectSend' method
23b7dc2 
this method can lator on be used to send a fidelity bond
to an external destination, renew the fidelity bond, or
generally send selected utxos without manually freezing/
unfreezing them before.
@theborakompanioni
refactor: externalize PaymentConfirmModal
ada3fce
@dergigi
Copy link
Contributor

dergigi commented Nov 9, 2022

Oh wow, that's big. Will do my best to review this fully until EOW.

@dergigi
Copy link
Contributor

dergigi commented Dec 9, 2022

Conflicts introduced due to merge of #566 - please resolve @theborakompanioni 🙏

@theborakompanioni
Copy link
Collaborator Author

Conflicts introduced due to merge of #566 - please resolve @theborakompanioni pray

Resolved. 🙏

@dergigi
Copy link
Contributor

dergigi commented Dec 9, 2022

What about: "Unlock expired fidelity bond?"

That's what you want to do after all, no? In some sense the FB is locked away, both in time and space. Once the timelock expires you can unlock it completely, by moving it to any jar.

Thoughts on this terminology @theborakompanioni @editwentyone?

@dergigi
Copy link
Contributor

dergigi commented Dec 9, 2022 •
edited
Loading

Another thought: Why give the user the option to select a jar in the first place?

I'd default to Jar A, or at least pre-select A and hide the selection below a fold. Why make the user think? It's not terribly important, is it? The user just wants to press "Unlock" and wants to use the funds again.

Here is how I would simplify the flow & the language:

Screenshot 2022-12-09 at 13-25-42 Jam for JoinMarket

Screenshot 2022-12-09 at 13-26-27 Jam for JoinMarket

Two clicks, the second one being a confirmation.

(I'd also remove the red warning that tells the user that this is a send without privacy improvement. The user can't change it at this point, there is no need to show a warning. It's just scary and confusing.)

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Dec 9, 2022 •
edited
Loading

Another thought: Why give the user the option to select a jar in the first place?

I'd default to Jar A, or at least pre-select A and hide the selection below a fold. Why make the user think? It's not terribly important, is it? The user just wants to press "Unlock" and wants to use the funds again.

Done.

Two clicks, the second one being a confirmation.

Not too convinced, as this is an action not performed very often and the described approach provides less freedom to users.
It is now changed to skip the jar selection and default to Jar A automatically - the code to select a Jar is still present for now and can be enabled with a flag - hope that is okay (in case it is decided to enable this flow or provide a toggle).

(I'd also remove the red warning that tells the user that this is a send without privacy improvement. The user can't change it at this point, there is no need to show a warning. It's just scary and confusing.)

Done.

@dergigi
Copy link
Contributor

dergigi commented Dec 13, 2022

Nice! I like it. Much more streamlined.

It is now changed to skip the jar selection and default to Jar A automatically - the code to select a Jar is still present for now and can be enabled with a flag - hope that is okay (in case it is decided to enable this flow or provide a toggle).

If we want to keep it in, we could add an Unlock Options to the Unlock modal, just like we have on Receive?

Add this "fold" ...

Screenshot 2022-12-13 at 11-53-26 Jam for JoinMarket

...to this screen:

Screenshot 2022-12-13 at 11-54-49 Jam for JoinMarket

Would still default to A (unless you go through the trouble of changing it) and would hide the complexity nicely, while still providing the option.

What do you think?

@theborakompanioni theborakompanioni mentioned this pull request Dec 13, 2022
Open
@theborakompanioni
Copy link
Collaborator Author

What do you think?

Full ACK.. Much better, imho. : -)
I'll merge this as it's an improvement to the current state and it feels "good enough" for now.
It is more important that users are able to spend their expired fidelity bonds. Currently, only acting as maker would freeze expired bonds automatically (in case it was unfrozen), but not when acting as a taker. This can potentially lead to confusion and/or needing additional support.

An issue for this has been created: #574

@theborakompanioni theborakompanioni merged commit 9f42dac into master Dec 13, 2022
@theborakompanioni theborakompanioni deleted the spend-fidelity-bond branch December 13, 2022 14:11
@dergigi dergigi mentioned this pull request Feb 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@editwentyone editwentyone editwentyone approved these changes

@dergigi dergigi Awaiting requested review from dergigi

Assignees

@theborakompanioni theborakompanioni

Labels
concept Wild idea, or too many details unknown yet enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theborakompanioni @editwentyone @dergigi